test, url: updated assertions in url-searchparams-getall tests #11142
Conversation
hiroppy
added
the
whatwg-url
| @@ -25,15 +25,17 @@ test(function() { | |||
|
|
|||
| test(function() { | |||
| var params = new URLSearchParams('a=1&a=2&a=3&a'); | |||
| assert_true(params.has('a'), 'Search params object has name "a"'); | |||
| assert_true(params.has('a'), `Search params object doesn't have name "a"`); | |||
There was a problem hiding this comment.
This will not pass make lint given that it is a plain string without any replacements. Please simply avoid the use of the contraction and make it a normal string.. e.g. 'Search params object does not have name "a"'.
There was a problem hiding this comment.
We disable eslint for WPT tests though, because the styles are too different.. :D
There was a problem hiding this comment.
Still better to keep it consistent
| assert_true(matches && matches.length == 4, 'Search params object has values for name "a"'); | ||
| assert_array_equals(matches, ['1', '2', '3', ''], 'Search params object has expected name "a" values'); | ||
| assert(matches); | ||
| assert_equals(matches.length, 4, `Unexpected length of name "a" values in search params object: ${matches.length}`); |
There was a problem hiding this comment.
Fixed in separate commit, I can squash them at the end.
|
@IvanJov, this specific test was imported directly from web-platform-tests, and we don't make any downstream changes. Can you submit the PR there instead? |
|
+1 on upstreaming the changes first. The tests are in their url directory, just send in a PR with the changes applied to a corresponding .html file would be enough I believe. |
|
@TimothyGu @joyeecheung Ah sorry, didn't know that. I am updating code on What about other tests, can you tell me what tests are also imported from some other repo? 🙂 Thanks! |
|
Generally the imported tests should have a comment mentioning the source (I remember the old I'll add a note to guides/writing-tests.md about this. |
|
@IvanJov Thanks for taking the time to work on Node.js! It looks like that test went through some significant modifications about a week after you and I discussed it. The part that I was referring to was removed in 10b687b and replaced with a port of the actual WPT test harness and tests. Sorry about that, and I hope you don't feel discouraged! If you want to try to find other places to contribute but you're not sure what to start with, http://nodetodo.org/next-steps/ might be helpful. |
|
Ah, didn't know it's a NodeTodo PR :D Sorry about the confusion. FWIW, I think the upstreaming process for WPT is not really a must, at least not as strict as we are about |
|
@Trott Thanks, I will find something else to work on! Thanks a lot for your help! |
|
Given that, I'm going to close this. We can reopen if necessary after changes have been upstreamed. |
Add guide on choice of assertions, use of ES.Next features, and the WPT upstream. PR-URL: nodejs#11150 Ref: nodejs#11142 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Add guide on choice of assertions, use of ES.Next features, and the WPT upstream. PR-URL: nodejs#11150 Ref: nodejs#11142 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Add guide on choice of assertions, use of ES.Next features, and the WPT upstream. PR-URL: nodejs#11150 Ref: nodejs#11142 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Add guide on choice of assertions, use of ES.Next features, and the WPT upstream. PR-URL: #11150 Ref: #11142 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Add guide on choice of assertions, use of ES.Next features, and the WPT upstream. PR-URL: #11150 Ref: #11142 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Add guide on choice of assertions, use of ES.Next features, and the WPT upstream. PR-URL: #11150 Ref: #11142 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Add guide on choice of assertions, use of ES.Next features, and the WPT upstream. PR-URL: #11150 Ref: #11142 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
I have updated assertions in url-searchparams-getall tests, made them more clear
for debugging and improved test messages.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
no